Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

main loop: plugins for periodic functions #3492

Merged
merged 19 commits into from
Apr 7, 2020

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Jan 30, 2020

Closes #3485
Redefines #2866 -> #3495
Facilitates #3329

Note: built atop #3489

Note: This is a first step, the interface may get generalised later. We may as well run all main loop functions through this interface (for logging, timing, etc).

Highlights

  • Convert main loop to asyncio
    • No performance improvement as the underlying code is still synchronous (see caveats).
  • Implement main loop plugins:
    • These run asynchronously at defined intervals at the end of the main loop.
    • Plugin executions are timed and logged (to logging.DEBUG level).
    • They can be turned on or off in the global/suite config or turned on via the CLI --main-loop arg.
    • Plugins use lazy loading so if you aren't using "auto restart" functionality it doesn't get imported.
  • Migrate "health check" functionality to two new plugins:
    • "health check" covers files-ystem integrity (contact file, suite run dir).
    • "auto restart" covers the "condemned hosts" functionality.
  • Add new plugins:
    • "log memory" monitors the memory usage of cylc.flow.scheduler.Scheduler attributes.
    • "log data store" monitors the number and memory usage of data store objects.
    • "log main loop" monitors the main loop plugins (very meta) produces a graph of plugin run times.
  • Shrink cylc.flow.scheduler down a bit - this module has got too bloated.

Caveats:

  • The main loop is asynchronous only in the loosest possible way, at least it is now possible to make bits of it run asynchronously, we can chip away at the synchronous code as and when (will update Replace scheduler.py main loop by asyncio loop #2866 to reflect this).
  • The main loop plugin interface is asynchronous but the plugins themselves are synchronous (meaning no performance enhancement), this is something we can improve as and when.
  • At present plugin functions cannot define their own configuration options. We can look at changing this after cfgspec modules have been refactored in build documentation into cylc.flow.cfgspec modules #3253 .

Examples:

Changes to make to your global config for testing:

 # if you don't specify an interval the plugin runs each main loop iteration
 # (except health check which has a hardcoded default in the global config)
 [cylc]
-    health check interval = PT5S
+    [[main loop]]
+        plugins = health check, auto restart
+        [[[health check]]]
+            interval = PT5S
+        [[[auto restart]]]
+            interval = PT10S
+        [[[log data store]]]
+            interval = PT2S
+        [[[log memory]]]
+            interval = PT1S

How to run a suite using extra plugins:

$ cylc run <suite> --main-loop 'log memory' --main-loop 'log data store'

How to write your own plugins:

from cylc.flow.main_loop import startup, periodic

@startup
def my_startup_function(sched, state): pass

@periodic
def my_periodic_function(sched, state): pass
--- a/setup.cfg
+++ b/setup.cfg
     log_memory = cylc.flow.main_loop.log_memory
+    my_plugin = cylc.flow.main_loop.my_module

Example plot from "log memory" (find it in the suite run dir after shutdown):

log_memory

Example plot from "log main loop" showing the execution times of different plugins:

log_main_loop

TODO:

  • Convert suite timeout (can do this whenever)
  • Convert suite stall (can do this whenever)
  • Changelog
  • Documentation

(awaiting cursory approval)

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • (master branch) I have opened a documentation PR at Main loop plugins cylc-doc#119.

@oliver-sanders oliver-sanders self-assigned this Feb 4, 2020
@oliver-sanders oliver-sanders added this to the cylc-8.0.0 milestone Feb 5, 2020
@oliver-sanders oliver-sanders marked this pull request as ready for review February 5, 2020 01:10
@oliver-sanders oliver-sanders force-pushed the main-loop-plugins branch 3 times, most recently from a810790 to 0292644 Compare February 5, 2020 02:10
@dwsutherland
Copy link
Member

dwsutherland commented Feb 6, 2020

This is great 👍

Will have a closer look soon.. Does the memory profile exclude recursive object references?

@oliver-sanders
Copy link
Member Author

Does the memory profile exclude recursive object references?

Yes, Pympler only counts objects once.

A simple example:

>>> print(asized([1, 2, 1], detail=2).format())
[1, 2, 1] size=152 flat=88
    1 size=32 flat=32
    2 size=32 flat=32
    1 size=0 flat=0

A recursive example:

>>> a = [1, 2, 3]
>>> b = [4, 5, 6]
>>> a.append(a)
>>> b.append(a)
>>> print(asized(b, detail=4).format())
[4, 5, 6, [1, 2, 3, [...]]] size=432 flat=120
    [1, 2, 3, [...]] size=216 flat=120
        1 size=32 flat=32
        2 size=32 flat=32
        3 size=32 flat=32
        [1, 2, 3, [...]] size=0 flat=0  # the recursive obj has a size of 0
    4 size=32 flat=32
    5 size=32 flat=32
    6 size=32 flat=32

(note recursive objects have a size of 0 because the pointer overheads belong to the flat component of the containing data structure)

However beware, Pympler only counts memory once, but it does so the first time it encounters an object which means you can get very different results for something which is fundamentally the same measurement:

>>> a = list(range(1000))
>>> b = ['x', 'y', a]
>>> c = ['z', a]

The list `b` is huge but `c` is tiny:
>>> print(asized([b, c], detail=1).format())
[['x', 'y', [0, 1, 2, 3, 4, 5, 6, 7, 8...., 993, 994, 995, 996, 997, 998, 999]]] size=41520 flat=80
    ['x', 'y', [0, 1, 2, 3, 4, 5, 6, 7, 8,....2, 993, 994, 995, 996, 997, 998, 999]] size=41304 flat=88
    ['z', [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 1....2, 993, 994, 995, 996, 997, 998, 999]] size=136 flat=80

Wait, what, now it's the other way around:
>>> print(asized([c, b], detail=1).format())
[['z', [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, ...., 993, 994, 995, 996, 997, 998, 999]]] size=41520 flat=80
    ['z', [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 1....2, 993, 994, 995, 996, 997, 998, 999]] size=41240 flat=80
    ['x', 'y', [0, 1, 2, 3, 4, 5, 6, 7, 8,....2, 993, 994, 995, 996, 997, 998, 999]] size=200 flat=88

In Cylc the main offender for this is the config object which will be associated with whatever object references it first.

@oliver-sanders
Copy link
Member Author

Adding the WIP label as this should to be generalised in light of the desired async main-loop endgame.

@oliver-sanders oliver-sanders force-pushed the main-loop-plugins branch 2 times, most recently from e4a1100 to d9f1106 Compare February 17, 2020 04:58
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Feb 17, 2020

Note:

  • Should have one test failure tests/database/05-lock-recover-100.t
  • Unittests are not being run ATM as the modules are [purposefully] missing the [superfluous] test_ prefix.
  • The plugin interface will change from before, during, after to @main_loop.before, .during, .after.

Remaining test failure turns out to be a faff, test suite behaves differently when run outside the battery.

Note: issue probably triggered by health check interval

@hjoliver
Copy link
Member

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 2
           

Complexity increasing per file
==============================
- cylc/flow/main_loop/log_memory.py  6
- cylc/flow/main_loop/log_data_store.py  7
- cylc/flow/main_loop/__init__.py  7
- cylc/flow/main_loop/health_check.py  3
- cylc/flow/host_select.py  15
- cylc/flow/main_loop/log_main_loop.py  5
- cylc/flow/main_loop/auto_restart.py  10
- cylc/flow/parsec/OrderedDict.py  8
- cylc/flow/scripts/cylc_psutil.py  4
         

Complexity decreasing per file
==============================
+ cylc/flow/scheduler_cli.py  -1
         

See the complete overview on Codacy

@oliver-sanders
Copy link
Member Author

Ok, I've added decorator functions for writing plugins:

from cylc.flow.main_loop import periodic

@periodic
def monitor_something(sched, state): pass

This should be forward compatible with the sort of stuff we may want to do later:

from cylc.flow.main_loop import task_event

@task_event
def monitor_task_event(task_event, state): pass

@oliver-sanders
Copy link
Member Author

Deconfilcted.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Apr 2, 2020

Rebased (should remove unit test failure)...
... (it did)

@oliver-sanders
Copy link
Member Author

@dwsutherland tagged you as a reviewer as you'd commented before, feel free to reassign if swamped by ops work, the graphql stuff is more important.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Apr 6, 2020

Deconflicted.

(plus some bonus documentation)

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliver-sanders, would it be possible to list the available plugins too?

For example, I was playing with the my_plugin from the docs, but couldn't figure out which key/name to use. It would be helpful to get a list of available plugins in the error message.

2020-04-07T10:46:32+12:00 ERROR - No main-loop plugin: "myplugin"
	Traceback (most recent call last):
	  File "/home/kinow/Development/python/workspace/cylc-flow/cylc/flow/main_loop/__init__.py", line 325, in load
	    module_name = entry_points[plugin_name.replace(' ', '_')]
	KeyError: 'myplugin'
	During handling of the above exception, another exception occurred:
	Traceback (most recent call last):
	  File "/home/kinow/Development/python/workspace/cylc-flow/cylc/flow/scheduler.py", line 306, in start
	    self.configure()
	  File "/home/kinow/Development/python/workspace/cylc-flow/cylc/flow/scheduler.py", line 558, in configure
	    self.options.main_loop
	  File "/home/kinow/Development/python/workspace/cylc-flow/cylc/flow/main_loop/__init__.py", line 327, in load
	    raise UserInputError(f'No main-loop plugin: "{plugin_name}"')
	cylc.flow.exceptions.UserInputError: No main-loop plugin: "myplugin"
2020-04-07T10:46:32+12:00 CRITICAL - Suite shutting down - No main-loop plugin: "myplugin"

Tested the log data store and health check plugins with the following added to my ~/.cylc/flow/8.0a1/flow.rc:

[cylc]
[[main loop]]
plugins = health check, log data store
[[[health check]]]
interval = PT10S
[[[log data store]]]
interval = PT10S
[[[my plugin]]]

Then setting breakpoints and looking at the console to confirm the code was executed.

image

Finally tested raising a ValueError in health check and confirmed that log data store was still executed successfully.

🎉 🚀

I couldn't use the my-plugin from the docs. Not sure what I am doing wrong? (left a comment in the code about it)

cylc/flow/main_loop/__init__.py Outdated Show resolved Hide resolved
cylc/flow/scheduler.py Outdated Show resolved Hide resolved
cylc/flow/main_loop/log_data_store.py Show resolved Hide resolved
cylc/flow/main_loop/__init__.py Show resolved Hide resolved
cylc/flow/main_loop/__init__.py Show resolved Hide resolved
cylc/flow/main_loop/__init__.py Show resolved Hide resolved
.. code-block:: console

$ # run a workflow using the "health check" and "auto restart" plugins:
$ cylc run my-workflow --main-loop 'health check' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the UI this gets rendered with some extra spaces. Should be fine I think.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, couldn't work out how to get around that without the docstring going over 79 chars.

additional_plugins = additional_plugins or []
entry_points = pkg_resources.get_entry_map(
'cylc-flow'
).get('main_loop', {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't use the my-plugin from the documentation. Debugging, the entry_points here never had my my-plugin, so that's never loaded correctly for me.

Is there anything I am missing @oliver-sanders ? I thought we would need something like this https://packaging.python.org/guides/creating-and-discovering-plugins/ (see part about namespaces)

Copy link
Member Author

@oliver-sanders oliver-sanders Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(in reply to @kinow and this comment from @dwsutherland)

Maybe there's a way of doing it without adding to setup.cfg?

I haven't yet implemented the mechanism required for external projects to add entrypoints to Cylc Flow. Not really sure how to go about that, so for now I've documented the entry point as cylc.main_loop.

The only experience I have with this sort of thing is with Pytest, which uses the entrypoint pytest11.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move that for later than 👍

@kinow
Copy link
Member

kinow commented Apr 6, 2020

Interesting new dependency added to Cylc Flow (optional I think for some plugins): https://pypi.org/project/kiwisolver/. Not sure where the name comes from, but the authors call it Kiwi. And quite sure one of the authors is the creator of Phosphor.JS (now Lumino).

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me 👍

I managed to get the my-plugin example working:

(cylc-doc) sutherlander@cortex-vbox:my_plugin$ more my_plugin.py 
from cylc.flow import LOG
from cylc.flow.main_loop import startup

@startup
async def my_startup_coroutine(schd, state):
   # write Hello <suite name> to the Cylc log.
   LOG.info(f'Hello {schd.suite}')
(cylc-doc) sutherlander@cortex-vbox:my_plugin$ more setup.py 
#!/usr/bin/env python

from setuptools import setup
# plugins must be properly installed, in-place PYTHONPATH meddling will
# not work.
setup(
    name='my-plugin',
    version='1.0',
    py_modules=['my_plugin'],
    entry_points={
       # register this plugin with Cylc
       'cylc.main_loop': [
         # name = python.namespace.of.module
         'my_plugin=my_plugin.my_plugin'
       ]
    }
)
(cylc-doc) sutherlander@cortex-vbox:my_plugin$ pip install -e .^C

Now plugin available:

(cylc-doc) sutherlander@cortex-vbox:cylc-doc$ python
Python 3.7.5 (default, Nov 20 2019, 09:21:52) 
[GCC 9.2.1 20191008] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import my_plugin
>>> dir(my_plugin)
['LOG', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__', 'my_startup_coroutine', 'startup']
>>>
(cylc-doc) sutherlander@cortex-vbox:my_plugin$ cd ../cylc-flow/
(cylc-doc) sutherlander@cortex-vbox:cylc-flow$ git diff
diff --git a/setup.cfg b/setup.cfg
index f745f0d01..9f0ca49ed 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -145,3 +145,4 @@ main_loop =
     log_data_store = cylc.flow.main_loop.log_data_store
     log_main_loop = cylc.flow.main_loop.log_main_loop
     log_memory = cylc.flow.main_loop.log_memory
+    my_plugin = my_plugin
(cylc-doc) sutherlander@cortex-vbox:cylc-flow$ pip install -e .["all"]^C

Maybe there's a way of doing it without adding to setup.cfg?
(otherwise should probably be in the docs... Thought that was the point of the above entry_points)

(cylc-doc) sutherlander@cortex-vbox:cylc-doc$ cylc run baz --main-loop 'my plugin'
            ._.                                                       
            | |                 The Cylc Suite Engine [8.0a1]         
._____._. ._| |_____.           Copyright (C) 2008-2019 NIWA          
| .___| | | | | .___|   & British Crown (Met Office) & Contributors.  
| !___| !_! | | !___.  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
!_____!___. |_!_____!  This program comes with ABSOLUTELY NO WARRANTY.
      .___! |              It is free software, you are welcome to    
      !_____!             redistribute it under certain conditions;   
                        see `COPYING' in the Cylc source distribution. 
                                                                       

*** listening on tcp://cortex-vbox:43015/ ***
*** publishing on tcp://cortex-vbox:43029 ***

To view suite server program contact information:
 $ cylc get-suite-contact baz

Other ways to see if the suite is still running:
 $ cylc scan -n 'baz' cortex-vbox
 $ cylc ping -v --host=cortex-vbox baz
 $ ps -wopid,args 25730  # on cortex-vbox

(cylc-doc) sutherlander@cortex-vbox:cylc-doc$ grep "Hello baz" /home/sutherlander/cylc-run/baz/log/suite/log
2020-04-07T16:46:01+12:00 INFO - Hello baz

When do we want to merge this?

@kinow
Copy link
Member

kinow commented Apr 7, 2020

Maybe there's a way of doing it without adding to setup.cfg?
(otherwise should probably be in the docs... Thought that was the point of the above entry_points)

That's what I thought. That users would need to just install the my-plugin project via pip to be able to use in Cylc Flow, without changing Cylc Flow's files.

@hjoliver
Copy link
Member

hjoliver commented Apr 7, 2020

When do we want to merge this?

I think we can merge it once @oliver-sanders has address the question about how to add a plugin. I also thought pip install my-plugin should be enough.

@oliver-sanders
Copy link
Member Author

It would be helpful to get a list of available plugins in the error message.

Good idea.

Done.

UserInputError: No main-loop plugin: "elephant"
    Available plugins:
        auto_restart
        health_check
        log_data_store
        log_main_loop
        log_memory

@oliver-sanders
Copy link
Member Author

Interesting new dependency added to Cylc Flow (optional I think for some plugins):

Yes I saw that! Wonderful Aus Vs NZ banter there.

This is dragged in by matplotlib, which is an optional dependency that wouldn't get installed for most uses.

@oliver-sanders
Copy link
Member Author

Great reviewing BTW, it's a big help.

@@ -42,6 +42,9 @@

Set a sensible interval before running suites.

If ``matplotlib`` is installed this plugin will plot results as a PDF in
the run directory when the suite is shut down (cleanly).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That cleanly note will be helpful too. I stopped the suite a few times with CTRL+C and went looking for the plot, then tried cylc stop five and that worked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add a suite abort hook too at some point.

@kinow
Copy link
Member

kinow commented Apr 7, 2020

Will check out code and re-run tests tomorrow morning. That should give time to others to have a play if they'd like too. Really good stuff coming with this PR! We need to post that somewhere like Discourse, some twitter, blog, etc. This is bringing plugins to Cylc in a way that I bet users will find useful and enjoy using.

@dwsutherland
Copy link
Member

Shouldn't cylc-flow be a dependency of cylc-doc now? (like in the setup.py)

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Apr 7, 2020

There was already a dependency between cylc-doc and cylc-flow. This dependency isn't in the setup.py on-purpose because you never (other than perhaps CI at release time) want pip to install cylc-flow for you when building cylc docs. Less automatic but makes you think about what you are actually documenting.

@kinow
Copy link
Member

kinow commented Apr 7, 2020

2 approvals, LGTM 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin: main loop extensions
5 participants